feat(auto-configuration-web): add configuration helper functions for web #2418
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2418 +/- ##
==========================================
- Coverage 90.97% 90.38% -0.60%
==========================================
Files 146 151 +5
Lines 7492 7302 -190
Branches 1502 1531 +29
==========================================
- Hits 6816 6600 -216
- Misses 676 702 +26 |
There was a problem hiding this comment.
@martinkuba overall structure looks promising 🙂 I do have a few questions I have around allowing multiple ways to set the same thing and the complexity that entails. 🤔 Having occasionally worked on NodeSDK, I've seen this get out of control quite quickly. Therefore I'd recommend doing an iterative approach (starting out with very few duplicate configurations, then adding more where necessary to make it simpler for the user). This can make maintenance easier going forward (and possibly also reduce user-confusion).
Another note: before we can continue we'll need to find owners that will be assigned to this package and will maintain it going forward.
| } | ||
|
|
||
| export function configureWebSDK( | ||
| config: TraceSDKConfiguration & EventSDKConfiguration, |
There was a problem hiding this comment.
could we do
| config: TraceSDKConfiguration & EventSDKConfiguration, | |
| config: {config: { traces: TraceSDKConfiguration, events: EventSDKConfiguration }, |
It makes resource be the odd-one out though as right now it's shared across both, but this would require the user to set both. 🤔
| * limitations under the License. | ||
| */ | ||
|
|
||
| export * from './utils'; |
There was a problem hiding this comment.
We've been gradually switching to explicit exports over all packages to avoid that we unintentionally add to the public API, I think we should also use explicit exports here.
| logRecordProcessors?: LogRecordProcessor[]; | ||
| logRecordExporter?: LogRecordExporter; |
There was a problem hiding this comment.
I wonder if we should allow either processors or exporters here, but not both 🤔
In general, I think the more we can reduce special cases where we need to auto-pair things/merge configs with other configs the better.
| if (!tracerConfig.resource && config.resource) { | ||
| tracerConfig.resource = config.resource; | ||
| } | ||
| const tracerProvider = new WebTracerProvider(tracerConfig); |
There was a problem hiding this comment.
Hmm, does it make it simpler for a user to have two places to specify the resource? 🤔
I wonder if we could be simplified to just take the resource from the tracerConfig if present without having too much impact on usability.
| }; | ||
| } | ||
|
|
||
| export function getResource(config?: ResourceConfiguration): Resource { |
There was a problem hiding this comment.
There's a specification on resource merging that states that when there's a resource provided, we may not auto-merge service.* into it even if it's not present. We should ensure that we align with that.
Co-authored-by: Marc Pichler <[email protected]>
Addresses open-telemetry/opentelemetry-js#4702
Alternative to open-telemetry/opentelemetry-js#4325
This is an alternative to creating a Web SDK package (similar to Node SDK). Instead of having an SDK class, this PR introduces helper functions for configuring different parts of web instrumentation.